[sisyphus-bot]

Review: feat: add Cynefin framework diagram type

Hey @ragelink — first off, thanks for taking this on. A text-based DSL for Cynefin is a genuinely novel contribution, and the amount of thought that went into the domain modeling (wavy boundaries, the cliff, confusion overlay, deterministic PRNG) is impressive. Sorry this sat without review for almost two weeks — let's get this across the finish line.

This is a substantial, well-structured PR. There are a few blocking items that need addressing before merge, but the architecture is sound and the path forward is clear. They are mostly admin pieces but for us they are important to keep the flow going.

What's working well

🎉 [praise] Langium grammar — correct choice for new diagrams, and the grammar is clean and expressive. The DOMAIN_NAME terminal constraining valid domain names at the grammar level is a smart design choice.

🎉 [praise] Deterministic boundary rendering — The seeded PRNG (cynefinBoundaries.ts) ensures reproducible wavy boundaries while still looking organic. Well-tested with proper determinism assertions.

🎉 [praise] Comprehensive test coverage for a first pass — DB unit tests, boundary path tests, full integration parsing tests (basic, transitions, accessibility, complex diagrams, clear-and-reparse), and multi-diagram ID uniqueness coverage. Strong foundation.

🎉 [praise] Clean diagram architecture — follows the radar/wardley pattern nicely: parser → db → renderer pipeline, proper clear() reset, Langium module registration, lazy-loaded detector. No cross-diagram imports.

🎉 [praise] Security — All user text inserted via D3's .text() (safe text nodes), no .html()/innerHTML, no string interpolation into SVG markup, Langium grammar constrains input. DOMPurify pipeline is intact. Clean bill of health.

Blocking items

🔴 [blocking] Missing Langium generated files
The Cynefin grammar is added to langium-config.json and module.ts imports CynefinGrammarGeneratedModule from ../generated/module.js, but the generated files in packages/parser/src/language/generated/ were not regenerated. The index.ts exports (Cynefin, isCynefin, isDomainBlock, isDomainItem, isTransition, CynefinGrammarGeneratedModule) don't exist in the current generated code. Please run:

pnpm --filter parser langium:generate

and commit the regenerated files.

🔴 [blocking] Missing Cypress e2e visual regression tests
The parser/DB coverage is solid. But new diagram types require visual regression tests in cypress/integration/rendering/ using the imgSnapshotTest() helper. This is one of the project's most important quality safeguards — it catches rendering regressions across themes and configurations. The test page (test-cynefin.html) already has 4 great test scenarios that could be adapted directly. Reference: any existing e2e test like cypress/integration/rendering/radar.spec.js.

🔴 [blocking] Missing changeset
No changeset file is included. For a new feature:

pnpm changeset
# Select: packages/mermaid, minor bump
# Message: feat: add Cynefin framework diagram type

Important items

🟡 [important] Renderer ignores config values
The config schema defines width, height, padding, showDomainDescriptions, and boundaryAmplitude — but the renderer hardcodes width = 800, height = 600, padding = 40 at cynefinRenderer.ts:72-74 and never reads from db.getConfig(). The showDomainDescriptions and boundaryAmplitude settings have no effect. It would be great to wire these up so users can actually configure the diagram.

🟡 [important] Renderer uses hardcoded colors instead of theme variables
cynefinRenderer.ts:55-61 defines DOMAIN_COLORS with hardcoded hex values, and item badges at lines 207-215 use hardcoded #ffffff, #cccccc, #333333. Meanwhile, all 5 theme files define cynefin-specific variables (complexBg, complicatedBg, chaoticBg, clearBg, confusionBg, textColor, labelColor) — but these are never consumed by the renderer or styles.ts. This means switching themes won't actually change domain colors. Worth wiring the theme variables through so the diagram respects theme selection.

🟡 [important] Missing documentation
New diagram types need docs in packages/mermaid/src/docs/. Use MERMAID_RELEASE_VERSION as the version placeholder in any "Available since" references. The syntax examples in the PR body and test-cynefin.html are a great starting point.

🟡 [important] test-cynefin.html committed to package root
packages/mermaid/test-cynefin.html looks like a development/debugging file. It uses securityLevel: 'loose' and imports from ./dist/. This probably shouldn't be committed — consider removing it or moving the test scenarios into the Cypress e2e tests instead.

Nits and suggestions

🟢 [nit] cynefinDb.ts:31,46 — setDomains(blocks: any[]) and setTransitions(transitions: any[]) use any[]. Consider using the Langium-generated AST types for stronger type safety.

💡 [suggestion] Handdrawn/rough mode — Not a blocker, but it would be good to explicitly note whether handdrawn mode is supported or not. If it can't be supported yet, a brief mention in the docs is sufficient so users know.

💡 [suggestion] config.type.ts regeneration — The changes to config.type.ts look correct and properly generated from the schema. Just double-check you ran pnpm run --filter mermaid types:build-config rather than editing manually — CI will verify this.

Self-check

• At least one 🎉 praise item (5 total)
• No duplicate comments
• Severity tally: 3 🔴 / 4 🟡 / 1 🟢 / 2 💡 / 5 🎉
• Verdict matches criteria: REQUEST_CHANGES (3 🔴 blocking)
• Not a draft PR
• Tone check: constructive and appreciative ✅

[sisyphus-bot]

Thanks for tackling this, @ragelink — and especially for the substantial follow-up work in commit ea1c48f that addressed prior review items (changeset added, Cypress e2e tests written, schema config wired through to the renderer, theme variables threaded across all five themes, dev test file removed, any[] replaced with proper Langium AST types, comprehensive syntax docs added). That kind of responsiveness on a large feature is exactly what makes new diagram types land. This PR is close to ready — most of what's below is final polish and a few correctness edges to tighten before flipping it on.

I'm doing a full New Diagram Type review per the project's contributor checklist, so this is structured rather than narrative. Good news first.

What's working well

🎉 Langium-first. cynefin.langium is the right choice for a new diagram (no JISON), and the grammar is concise and readable. The fixed DOMAIN_NAME terminal is a smart constraint — it prevents typos at parse time and gives the renderer a closed enum to switch on.

🎉 Plugin contract is complete. cynefinDetector.ts (id + detector + loader), cynefinDiagram.ts (parser + db + renderer + styles), cynefinDb.ts, cynefinParser.ts, cynefinRenderer.ts, styles.ts, types.ts — every piece is present and registered in diagram-orchestration.ts:32-115. Examples wired in (packages/examples/src/index.ts), cspell entry added, langium-config entry added, parser exports + parse-dispatcher added in packages/parser/src/parse.ts. Nothing missed in the wiring.

🎉 Excellent test coverage. 19 unit tests for DB/PRNG/path generators in cynefin.spec.ts, 13 integration tests covering parse-to-DB pipeline (basic, transitions, accessibility, complex, re-parse-after-clear) in cynefin.integration.spec.ts, and 8 Cypress visual regression scenarios in cypress/integration/rendering/cynefin.spec.js (basic, transitions, empty, many items, config override, theme override, zero amplitude, accessibility). The Cypress coverage is exactly what the project asks for on new diagrams.

🎉 Theme integration across all five themes. theme-base.js, theme-dark.js, theme-default.js, theme-forest.js, theme-neutral.js each define a complete cynefin block with domain backgrounds, cliff color, boundary color, and font sizes. Color choices visibly reflect each theme's palette (forest has earth tones, dark has saturated brights). This is the part new diagrams most often skimp on, and you didn't.

🎉 Schema-driven config done right. New optional CynefinDiagramConfig in config.schema.yaml with width, height, padding, showDomainDescriptions, boundaryAmplitude. All optional with sensible defaults — non-breaking. Properly threaded through db.getConfig() → renderer at cynefinRenderer.ts:84-93. The Cypress test at cynefin.spec.js:78-87 actually exercises the config override path. Beautiful.

🎉 Determinism via seeded PRNG. seededRandom (mulberry32) + hashString(id) means the wavy boundaries are stable per diagram id. Critical for visual snapshot testing — and the cynefin.spec.ts tests assert the determinism explicitly. Smart.

🎉 Comprehensive docs. packages/mermaid/src/docs/syntax/cynefin.md is 213 lines including conceptual intro, syntax reference, transition examples, configuration table, theme-variable table, accessibility section, and MERMAID_RELEASE_VERSION placeholder usage. Linked sidebar entry added.

Things to address

🟡 [important] — Self-loop transitions produce NaN coordinates

packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:326-356

The grammar allows complex --> complex (the Transition rule has no constraint that from and to differ). When that happens, the renderer computes:

const dx = x2 - x1;       // 0
const dy = y2 - y1;       // 0
const len = Math.sqrt(dx * dx + dy * dy);  // 0
const nx = -dy / len;     // NaN
const ny = dx / len;      // NaN
const cpx = mx + nx * offsetAmount - padding;  // NaN
const cpy = my + ny * offsetAmount - padding;  // NaN

The resulting path string M{cx},{cy} Q{NaN},{NaN} {cx},{cy} will silently fail or render an invisible path in most browsers, and the optional label gets placed at NaN. Since users could plausibly draw "self-reflection" transitions on the canonical Cynefin diagram, this is worth a defensive guard. Either:

1. Reject self-loops at the grammar/DB level with a helpful error, or
2. In the renderer, special-case len === 0 to either skip the transition or draw a small loopback arc above the domain center.

Whichever fits the conceptual model of the diagram better. Test case for either approach should join the integration suite.

🟡 [important] — Item badge width estimated heuristically; will misalign with real labels

packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:276

const textLen = item.label.length * 7 + itemPaddingX * 2;

The 7-pixels-per-character estimate is a rough average — fine for short ASCII strings, but it will produce visibly wrong badge widths for:

• Wide characters (W, M, asian glyphs)
• Narrow characters (i, l)
• Different fonts / font sizes (the badge font is configurable via theme)
• Strings shorter than ~3 chars (badges become awkwardly small)

The standard Mermaid pattern is to render the text first, call getBBox() to measure, then size the wrapping rect. See journeyRenderer.js or pieRenderer.ts for examples — the helper renders text invisibly, measures it, then positions the background rect around the measured bounds. Worth investing in the getBBox approach now since item badges are the most prominent UI element of the diagram and contributors will copy this pattern.

🟡 [important] — Confusion domain item placement spills outside the visible ellipse

packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:171-181, 264-303

The confusion ellipse is sized to roughly 10% of width × 10% of height (confusionRx = width * 0.1, confusionRy = height * 0.1 — at the default 800×600, that's an 80×60 ellipse). But items placed in the confusion domain start at layout.cy + (showDomainDescriptions ? 25 : 15) and stack downward at 30px per badge — meaning even one item will extend below the ellipse, and any more than that will appear floating in dead space at the diagram's center.

A few directions to consider:

1. Use a larger confusion region (the canonical Cynefin diagram has confusion as a real central area, not a small overlay)
2. Constrain the confusion-domain item count visually (collapse to "+N more" or similar)
3. Render confusion items differently (e.g., as a stacked list outside the ellipse with a connector)

Whichever direction fits the design intent — the current behavior will look broken with realistic content (and the empty-domain Cypress test passes precisely because no confusion items are placed).

🟡 [important] — Inline .attr() styling duplicates the styles.ts CSS

packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:135-181, 220-242, 281-301, 351-374 and styles.ts:1-104

The renderer applies presentation via D3 attribute calls (.attr('fill', ...), .attr('stroke', ...), .attr('font-size', ...), etc.) and the diagram ships a styles.ts that emits CSS for selectors like .cynefinDomainLabel, .cynefinSubtitle, .cynefinItem text, .cynefinArrowLine, etc. The CSS is largely shadowed by the inline attributes — for example, .cynefinDomainLabel { fill: ... } in styles.ts is overridden by the .attr('fill', theme.labelColor) inline at cynefinRenderer.ts:196.

This double-styling makes future theme changes confusing (which one wins? where do I edit?). Pick one model:

• CSS classes only (preferred — matches the rest of the codebase): keep styles.ts, drop the inline .attr('fill', ...) / .attr('font-size', ...) calls, and let the CSS handle presentation. Reference: journey/styles.js, pie/styles.ts.
• Inline attrs only: drop styles.ts entirely. Less consistent with the project but simpler. Not recommended.

Going with CSS classes also makes the diagram play more nicely with user CSS overrides (which is a documented Mermaid extension point).

🟡 [important] — Handdrawn / rough mode not addressed

The New Diagram Type checklist asks that handdrawn mode either be implemented or explicitly documented as unavailable. The cynefin docs (packages/mermaid/src/docs/syntax/cynefin.md) don't mention rough mode. Given the wavy organic boundaries are already a "handdrawn-feeling" aesthetic, this might be a case where the diagram is implicitly handdrawn-only — but that should be made explicit in the docs (one-liner stating handdrawn mode is not separately supported, with a brief reason).

Per the checklist this is "not a hard blocker for merging" but does need explicit acknowledgement.

🟢 [nit] — aria-label on root SVG vs <title> element

packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:111-113

Other Mermaid diagrams typically attach accessibility text via <title> and <desc> SVG child elements (the W3C recommendation for SVG accessibility). The cynefin renderer uses aria-label for the title and <desc> for the description — half and half. Consider switching the title to a <title> child element for consistency with diagrams like pieRenderer.ts. Not a security or functional issue (the XSS sub-agent confirmed accTitle is pre-sanitized via sanitizeText in commonDb.ts), just consistency.

🟢 [nit] — Grammar entry rule has three alternatives for the type marker

packages/parser/src/language/cynefin/cynefin.langium:5

('cynefin-beta' | 'cynefin-beta:' | 'cynefin-beta' ':')

The third alternative ('cynefin-beta' ':') accepts cynefin-beta : with a space before the colon. Three alternatives for a type marker is unusual — most diagrams accept only the bare token. If both cynefin-beta and cynefin-beta: need support, two alternatives should suffice. Worth a short comment in the grammar explaining the intent, or simplifying to one form (other Langium-based diagrams like radar and treemap use just the bare keyword).

🟢 [nit] — Detector regex has no end boundary

packages/mermaid/src/diagrams/cynefin/cynefinDetector.ts:10

return /^\s*cynefin-beta/.test(txt);

Would also match cynefin-betaXYZ or cynefin-beta-extension. Other diagram detectors have the same quirk so this is consistent, but a \b word boundary or (?:\s|$) lookahead would be more precise. Take it or leave it.

🟢 [nit] — getCynefinTheme casts via any

packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:53-75

return (themeVariables as any).cynefin as { /* ... */ };

The shape is locally typed (good), but the any cast loses the connection to the theme system. If you can extend the MermaidThemeVariables interface with an optional cynefin?: CynefinThemeVariables field, you'd avoid the cast. Existing diagrams (e.g., radar, venn) augment the theme typings — worth following that pattern.

💡 [suggestion] — Use Resolves #7534 instead of Ref: #7534 in the PR description

GitHub auto-closes the linked issue when the PR merges if you use Resolves/Closes/Fixes. With Ref: someone has to close it manually. Tiny thing.

💡 [suggestion] — Item overflow handling

Long item labels or many items per domain will overflow the domain bounds. Consider word-wrapping, text truncation with ellipsis, or scaling. Not blocking — first beta is allowed to be opinionated about content fitting — but worth a TODO comment or follow-up issue so it doesn't get lost.

💡 [suggestion] — Constraint: transitions to/from confusion

Architecturally, "confusion" represents not knowing which domain an item belongs to, so transitions like complex --> confusion are conceptually questionable (the canonical model has items move OUT of confusion, not into it). Worth a sentence in the docs about which transitions are conceptually meaningful, or constraining the grammar to allow only confusion --> X. Doesn't have to be done now.

Security

✅ No XSS or injection issues identified. The XSS sub-agent reviewed all 12 changed files in the cynefin diagram + parser. All user-controlled text (item.label, transition.label, title, accDescription) flows through D3's .text() method (text-node creation, not HTML parsing). accTitle is set via .attr('aria-label', ...) after pre-sanitization in commonDb. No innerHTML, .html(), eval, Function(), foreignObject, event handlers, <script>, or javascript: URIs introduced. The styles.ts CSS interpolation follows the same pattern as existing diagrams — pre-existing accepted approach, not a new attack surface. The diagram does not bypass the standard mermaidAPI render pipeline, so DOMPurify sanitization still applies to final SVG output.

Verdict

Requesting changes for the 4 🟡 items in aggregate (each individually borderline, but together they shape the visual quality and correctness of the diagram's first beta). The handdrawn-mode acknowledgement is the cheapest fix; the confusion-overflow and self-loop NaN are the most user-visible. Once those land, this should be in great shape for the first text-based Cynefin DSL anywhere.

Genuinely impressive scope for a single PR — Langium grammar + renderer + 5 themes + schema + 40 tests + comprehensive docs. Looking forward to the next iteration.

Self-check

• 🔴 blocking: 0
• 🟡 important: 4
• 🟢 nit: 4
• 💡 suggestion: 3
• 🎉 praise: 6

A word from @knsv. Great progress, just a few small things left. The diagrams look great!

[sisyphus-bot]

Thanks so much for this contribution, @ragelink — a first-of-its-kind text DSL for Cynefin is a lovely thing to land in Mermaid, and the quality of the work shows throughout. I went through the parser, renderer, boundaries, themes, schema, tests, and docs, plus a focused security pass. Great shape overall; a few small things below to iron out before merge.

🎉 What's working well

• 🎉 Type-safe grammar. DOMAIN_NAME is restricted to the five canonical literals in packages/parser/src/language/cynefin/cynefin.langium, which means domain lookups in the renderer (domainBg[domainName], layouts[…]) are guaranteed safe by the parser. Elegant.
• 🎉 Separation of concerns. Boundary path math lives in its own module (cynefinBoundaries.ts) with a deterministic seeded PRNG — reproducible output across renders and easy to unit-test.
• 🎉 Defense in depth for self-loops. Filtered in cynefinDb.ts:setTransitions and also guarded in cynefinRenderer.ts inside the transition loop.
• 🎉 Theme integration is thorough. All five themes (default/dark/forest/neutral/base) define a cynefin block with overridable domain colors, boundary/cliff widths, font sizes, etc. The dark and forest themes supply genuinely different palettes.
• 🎉 Tests are solid. 162 lines of DB unit tests + 275 lines of parser integration tests + 10 Cypress visual cases (including config overrides, theme overrides, accessibility, straight vs wavy boundaries). This is well above the bar for a new diagram type.
• 🎉 Multi-diagram uniqueness test updated. The entry in multi-diagram-id-uniqueness.spec.ts is exactly what we want for scoped marker IDs like cynefin-arrow-${id}.
• 🎉 Changeset + MERMAID_RELEASE_VERSION placeholder in docs — followed the conventions precisely.
• 🎉 Thoughtful UX touches. The overflow badge (+N more) when the confusion ellipse has more than MAX_CONFUSION_ITEMS, and the accessibility <title>/<desc> elements.

Security

I ran a focused XSS/injection review over the renderer, DB, parser, boundaries, and styles. No XSS or injection issues identified. All user-controlled text (item labels, transition labels, title, accTitle, accDescription, overflow badge) flows to D3's .text() (sets textContent, which HTML-escapes). No .html()/innerHTML, no foreignObject, no xlink:href/javascript: sinks, no DOMPurify config changes. Title/accTitle/accDescription additionally pass through sanitizeText upstream in commonDb.ts. The styles.ts CSS generator interpolates theme variables only, not diagram input.

Things to address

🟡 [important] useMaxWidth config is ignored

packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts passes true literally:

configureSvgSize(svg, totalHeight, totalWidth, true);

CynefinDiagramConfig extends BaseDiagramConfig, which supplies useMaxWidth, so users currently can't disable it. The radar renderer (which the PR notes as the reference pattern) reads from config — see packages/mermaid/src/diagrams/radar/renderer.ts:57:

configureSvgSize(svg, totalHeight, totalWidth, config.useMaxWidth ?? true);

Worth matching that so the config option actually works.

🟡 [important] Config key doesn't match the -beta keyword

The diagram is introduced via cynefin-beta at the syntax level, but the detector id and config key are cynefin (no -beta):

• packages/mermaid/src/diagrams/cynefin/cynefinDetector.ts:7 — const id = 'cynefin';
• packages/mermaid/src/schemas/config.schema.yaml — cynefin: $ref: '#/$defs/CynefinDiagramConfig'
• cynefinDb.ts reads commonGetConfig().cynefin

Compare to wardley-beta (the other -beta diagram in the codebase) — packages/mermaid/src/diagrams/wardley/wardleyDetector.ts:7 uses const id = 'wardley-beta'; and the schema key is wardley-beta:. The convention is: keyword == detector id == config/schema key.

Two concrete effects:

1. A user writing %%{init: {"cynefin-beta": {...}}}%% (by analogy with wardley-beta) will silently get no config applied.
2. When the -beta suffix is dropped in a later release, the schema key won't need to change but the detector id and grammar keyword will — the current asymmetry makes that migration more confusing, not less.

Easiest fix: rename detector id, schema key, and config property to cynefin-beta to match the keyword. (If the intent was specifically to avoid renaming the config block when going GA, please add a short comment to that effect on the schema entry so the inconsistency is discoverable.)

💡 [suggestion] Transition control-point coordinate math is fiddly

In cynefinRenderer.ts (transitions section), x1/y1/x2/y2 and mx/my are computed in padded SVG coords, then cpx/cpy subtract padding to land in root-local coords to match the M/Q path which uses unpadded fromLayout.cx,cy. It works, but a future reader has to hold two coordinate spaces in their head. Computing everything in root-local (drop the + padding on x1…y2, drop the - padding on cpx/cpy) would be algebraically equivalent and easier to follow.

💡 [suggestion] Overflow badge only for the confusion ellipse

MAX_CONFUSION_ITEMS = 3 caps confusion with a +N more badge, but the four quadrant domains can visually overflow their boxes with a long list. Not a blocker for v1 — the wavy boundaries are forgiving — but worth either applying the same pattern or documenting the expectation that quadrant item counts should be kept small.

💡 [suggestion] PR description test-plan checkboxes are stale

The PR body lists Cypress e2e and Cynefin unit/integration tests as unchecked ("happy to add if requested"), but cypress/integration/rendering/cynefin.spec.js, cynefin.spec.ts, and cynefin.integration.spec.ts are all present. A quick edit of the description will avoid a reviewer chasing those down.

Really excited about this one — let's get it across the finish line. 🙌

[sisyphus-bot]

Hey @ragelink — final pass to formally close this out. No new findings; the prior rounds covered everything substantive, and nothing has shifted under the Cynefin code since @knsv approved on April 29 (only the merges from develop and a couple of CI-scoping fix-ups).

Worth restating why this lands well, because four review rounds in it's easy to lose sight of the win:

• 🎉 Genuinely novel — first text-based DSL for Cynefin anywhere.
• 🎉 Clean architecture: Langium grammar with a closed DOMAIN_NAME enum, deterministic seeded PRNG behind the wavy boundaries in their own module, and full theme integration across all five themes.
• 🎉 Solid test coverage end-to-end — DB unit tests, parser integration tests, boundary determinism, multi-diagram ID uniqueness, and Cypress visual cases including the cliff and confusion overlay.
• 🎉 The persistence across multiple review rounds — including chasing the Argos and CI fix-ups at the end — is exactly the kind of follow-through that makes new diagram types ship. Genuinely appreciated.

To answer your "what happens next" question: nothing further needed from you. The PR is approved and a maintainer will merge it when the timing fits — likely bundled with the next mermaid minor bump given this is a feat: change. You can ignore the queue from here; if develop shifts under you and a conflict shows up, someone will ping you. Otherwise sit back and watch it land.

Thanks again for the stamina on this one. 🎉